-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add contribution guidelines #5
base: master
Are you sure you want to change the base?
Add contribution guidelines #5
Conversation
during the review process | ||
``` | ||
|
||
Commits relating to multiple files should follow these guidelines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of disagree we should treat commits using multiple files or single files differently. Looks like it could generate some inconsistency. Also I don't think there's any reason to have different max header length for these scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a limit on max header length because GitHub itself even recommends that length, and will trim messages that it considers "too long".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no yea, I'm completely ok with having a limit, my point is this limit being different for single file commits and multiple files commit. could be the same limit for both :)
```markdown | ||
Apply changes to files as per guidelines | ||
|
||
Changed Files: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Doesn't git itself provide this outside of the commit message? Secondly, most this repo will consist of documents, not code, I don't think we need to be very strict with the body of the message as long as the header is clear. Or that some description is provided somewhere, such as in the PR description. To be frank, I would rather have a longer description at the PR than at each commit. I know this is not great for git history, but we are heavily depending on github and not just git already anyway because of the issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't GitHub auto-populate the PR with the contents of the commit? My goal here is to avoid someone finding a segment of documentation that was added however long ago, and having to look back for a closed PR instead of being able to look directly at the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if github does that? This PR has empty description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on Slack, it puts the first line in as the title, and the rest of the lines - which in this case didn't exist - as the description.
Might be good to add something about using the imperative present on the header of commits with an example? Add is ok, but not Added or Adds Something else to have in mind is that contributing guidelines should also handle contributing behavior such as "be patient, everyone here is a volunteer" and even have a code of conduct. This, of course, doesn't mean that we can't have these guidelines first and then add a CoC later. I'm just mentioning because I think it's important to have this in mind, and maybe you also have some ideas about that. |
I'm actually not really good with setting up a code of conduct, but I've heard the Rust one is good. I will add a placeholder for now. |
c42da48
to
5a329c8
Compare
|
||
Commits relating to one file should follow these guidelines: | ||
|
||
- The header line should contain the filename and a short description of what |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not sure if enforcing the filename is ideal? I'll let others comment on this. Ideally we should make this more simple I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard to prepare release notes without this requirement. OTOH, we're not releasing software from this repo...
Yea, I'm happy to accept this PR with just a placeholder file for the code of conduct. I can work on this part later after I'm done editing the manifesto to comply with this |
|
||
Contributions to documents for the repository should follow these guidelines: | ||
|
||
- Lines should be no more than 80 characters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It makes the document harder to edit, imo. All text editors support text wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vim hard-wraps (at least in my case) all Git commits, and as discussed for the sole purpose of this pull-request, GitHub sucks at showing the context of a line. If someone types out a whole paragraph, sure, GitHub wraps it. However, you'll end up saying something along the lines of "four sentences in, there's a typo" rather than just commenting on one 80-character line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second the 80 characters limit. Otherwise diffs get difficult to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, .editorconfig
would be nice to include along with the guidelines.
This document serves as contribution guidelines and the points listed should be | ||
followed for contributions to the foundation. | ||
|
||
Contributions are expected to follow the [code of conduct](CODE_OF_CONDUCT.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop this line until there is a content in that file. No point in confusing the reader with illusory complexity.
I suggest to postpone making this official until we have real problems. We're editing documents, not code here. Git / GitHub process is already difficult. If we see a bad PR, we can always point a person to this PR for guidance. When we need to do that more than three times overall, then let's reconsider and maybe merge. This means that the README change has either to be dropped, or separated to another PR and merged, otherwise it will be a nightmare to rebase --- README will evolve. |
No description provided.